Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add per net device metrics #4145

Merged
merged 13 commits into from
Oct 18, 2023
Merged

feat: Add per net device metrics #4145

merged 13 commits into from
Oct 18, 2023

Conversation

wearyzen
Copy link
Contributor

@wearyzen wearyzen commented Oct 2, 2023

Changes

  • Move NetDeviceMetrics from logger to Net module.
  • Use NET_DEV_METRICS_PVT instead of adding an entry of NetDeviceMetrics in Net so that metrics can be flushed even from signal handlers.
  • use individual metrics instead of METRICS.net.
  • Use "net_$iface_id" for the metrics name instead of "net_$tap_name" to be consistent with the net endpoint "/network-interfaces/{iface_id}".

Reason

  • To add more granularity in analysing network device metrics.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following
Developer Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.

PR Checklist

  • [ ] If a specific issue led to this PR, this PR closes the issue.
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • [ ] API changes follow the Runbook for Firecracker API changes.
  • User-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.
  • [ ] New TODOs link to an issue.
  • Commits meet contribution quality standards.

  • This functionality cannot be added in rust-vmm.

@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (4a08802) 82.99% compared to head (42d52c0) 83.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4145      +/-   ##
==========================================
+ Coverage   82.99%   83.00%   +0.01%     
==========================================
  Files         221      222       +1     
  Lines       28567    28634      +67     
==========================================
+ Hits        23709    23769      +60     
- Misses       4858     4865       +7     
Flag Coverage Δ
4.14-c7g.metal 78.60% <97.94%> (+0.05%) ⬆️
4.14-m5d.metal 80.37% <97.94%> (+0.03%) ⬆️
4.14-m6a.metal 79.53% <97.94%> (+0.05%) ⬆️
4.14-m6g.metal 78.60% <97.94%> (+0.05%) ⬆️
4.14-m6i.metal 80.37% <97.94%> (+0.04%) ⬆️
5.10-c7g.metal 81.51% <97.94%> (+0.04%) ⬆️
5.10-m5d.metal 83.05% <97.94%> (+0.03%) ⬆️
5.10-m6a.metal 82.30% <97.94%> (+0.04%) ⬆️
5.10-m6g.metal 81.51% <97.94%> (+0.04%) ⬆️
5.10-m6i.metal 83.04% <97.94%> (+0.03%) ⬆️
6.1-c7g.metal 81.51% <97.94%> (+0.04%) ⬆️
6.1-m5d.metal 83.05% <97.94%> (+0.03%) ⬆️
6.1-m6a.metal 82.30% <97.94%> (+0.04%) ⬆️
6.1-m6g.metal 81.51% <97.94%> (+0.04%) ⬆️
6.1-m6i.metal 83.04% <97.94%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/vmm/src/devices/mod.rs 88.88% <100.00%> (ø)
src/vmm/src/devices/virtio/net/mod.rs 50.00% <ø> (ø)
src/vmm/src/logger/metrics.rs 96.20% <100.00%> (-0.21%) ⬇️
src/vmm/src/devices/virtio/net/device.rs 93.80% <98.18%> (+0.13%) ⬆️
src/vmm/src/devices/virtio/net/event_handler.rs 79.48% <0.00%> (ø)
src/vmm/src/devices/virtio/net/metrics.rs 98.70% <98.70%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wearyzen wearyzen marked this pull request as ready for review October 11, 2023 17:06
@wearyzen wearyzen self-assigned this Oct 11, 2023
@wearyzen wearyzen added the Status: Awaiting review Indicates that a pull request is ready to be reviewed label Oct 11, 2023
roypat
roypat previously requested changes Oct 12, 2023
src/vmm/src/devices/virtio/net/metrics.rs Outdated Show resolved Hide resolved
@wearyzen wearyzen marked this pull request as draft October 13, 2023 18:26
@wearyzen wearyzen marked this pull request as ready for review October 15, 2023 13:18
src/vmm/src/devices/virtio/net/metrics.rs Outdated Show resolved Hide resolved
src/vmm/src/devices/mod.rs Outdated Show resolved Hide resolved
src/vmm/src/devices/virtio/net/metrics.rs Outdated Show resolved Hide resolved
src/vmm/src/devices/virtio/net/metrics.rs Outdated Show resolved Hide resolved
src/vmm/src/devices/virtio/net/metrics.rs Outdated Show resolved Hide resolved
src/vmm/src/logger/metrics.rs Outdated Show resolved Hide resolved
src/vmm/src/logger/metrics.rs Outdated Show resolved Hide resolved
src/vmm/src/logger/metrics.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@roypat roypat dismissed their stale review October 16, 2023 10:45

No more thread-safety issues

Copy link
Contributor

@ShadowCurse ShadowCurse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems the implementation jumps all over the place for this task, but I guess it is ok.

src/vmm/src/devices/virtio/net/metrics.rs Outdated Show resolved Hide resolved
src/vmm/src/devices/virtio/net/metrics.rs Outdated Show resolved Hide resolved
@wearyzen wearyzen requested a review from ShadowCurse October 17, 2023 11:12
Sudan Landge added 12 commits October 17, 2023 15:25
Emit aggregate and per device metrics for network devices:
 - Move NetDeviceMetrics from logger to Net module.
 - Use NET_DEV_METRICS_PVT instead of adding an entry of
   NetDeviceMetrics in Net so that metrics can be flushed
   even from signal handlers.

Note: this is part 1 of 12 commits to add per net device metrics.

Signed-off-by: Sudan Landge <sudanl@amazon.com>
Use individual instance of metrics instead of the METRICS.net.
Since we do not use METRIC.net, modify report_net_event_fail to
use self.metrics.

Note: this is part 2 of 12 commits to add per net device metrics.

Signed-off-by: Sudan Landge <sudanl@amazon.com>
Update CHANGELOGS to notify that Firecracker now emits
per net device metrics.

Note: this is part 3 of 12 commits to add per net device metrics.

Signed-off-by: Sudan Landge <sudanl@amazon.com>
Create a unit test that allocates 19 NetDeviceMetrics
(19 because it is the max number of irqs we have for
net devices), update and verify a metric.

Note: this is part 4 of 12 commits to add per net device metrics.

Signed-off-by: Sudan Landge <sudanl@amazon.com>
Use RwLock to make accessing Metrics threadsafe.
Use better name to allocate NetDeviceMetrics

Note: this is part 5 of 12 commits to add per net device metrics.

Signed-off-by: Sudan Landge <sudanl@amazon.com>
With Vec based approach, "net0" is metrics for the 1st net device
created. However, if devices are not created in the same order all
the time, the first device created could either be eth0 or eth1 and
then net0 could sometimes point to eth0 and sometimes to eth1 which
doesn't help with analysing the metrics.
So, use Map instead of Vec to provide more clarity on which interface
the metrics actually belongs to.
Also, instead of net0 name the metrics json object as net_$iface_id
e.g. net_eth0 or net_eth1 which should help in better analysis.
Use "net_$iface_id" for the metrics name instead of "net_$tap_name" to
be consistent with the net endpoint "/network-interfaces/{iface_id}"

Note: this is part 6 of 12 commits to add per net device metrics.

Signed-off-by: Sudan Landge <sudanl@amazon.com>
Use proper convention of lower case for macros.
Make CHANHELOG less verbose.

Note: this is part 7 of 12 commits to add per net device metrics.

Signed-off-by: Sudan Landge <sudanl@amazon.com>
NetMetricsSerializer naming was confusing because its not a
Serializer in serde's sense (it implements Serialize, not Serializer).
So give it to a better name.

Note: this is part 8 of 12 commits to add per net device metrics.

Signed-off-by: Sudan Landge <sudanl@amazon.com>
NetDeviceMetrics was moved from logger which needed the new()
because default didn't provide the const implementation that
FirecrackerMetrics::new() needed. But since that is not a requirement
anymore we rely on default to provide 0 initialized NetDeviceMetrics.

NetMetricsSerializeProxy doesn't have any fields and so there is no
need to define new() for it.

Note: this is part 9 of 12 commits to add per net device metrics.

Signed-off-by: Sudan Landge <sudanl@amazon.com>
Better implementation for flush_metrics as suggested in feedback.

Note: this is part 10 of 12 commits to add per net device metrics.

Signed-off-by: Sudan Landge <sudanl@amazon.com>
Make sure there is no breaking change for NetDeviceMetrics.
Add random number of net devices and validate that values of
"net" are aggregate of all "net_{iface_id}".

Note: this is part 11 of 12 commits to add per net device metrics.

Signed-off-by: Sudan Landge <sudanl@amazon.com>
Use Arc for NetDeviceMetrics since this allows us to store
NetDeviceMetrics directly inside Net and get rid of the
net_metrics! macro.
Also, use better name for net metrics.

Note: this is part 12 of 12 commits to add per net device metrics.

Signed-off-by: Sudan Landge <sudanl@amazon.com>
@wearyzen wearyzen requested a review from roypat October 18, 2023 08:48
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@wearyzen wearyzen requested a review from kalyazin October 18, 2023 11:18
Copy link
Contributor

@kalyazin kalyazin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changelog update looks good to me

@wearyzen wearyzen merged commit 00524a7 into firecracker-microvm:main Oct 18, 2023
5 checks passed
@wearyzen wearyzen deleted the perdev_net_metrics branch January 17, 2024 20:53
@wearyzen wearyzen restored the perdev_net_metrics branch January 17, 2024 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Awaiting review Indicates that a pull request is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants